Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

test($sanitize): skip clobber test on Edge #16388

Merged
merged 3 commits into from
Jan 8, 2018

Conversation

Narretz
Copy link
Contributor

@Narretz Narretz commented Jan 3, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

$sanitize('<form><input name="parentNode" /></form>');
}).toThrowMinErr('$sanitize', 'elclob');
if (!/Edge\/16/.test(window.navigator.userAgent)) {
// Skip test on Edge 16 due to browser bug.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is that the issue is private, so it goes to 404. Maybe we should create an issue in our repo with a short description?

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just occurred to me that on Edge, when this fails to fail, the return value from $sanitize appears to be something like the console object.
Am I right?
How about we change the code so that if the result that will be returned from $sanitize is not a string then we throw the elclob error?
That way we don’t need to change our tests and we are covered from returning something dodgy in Edge.

@Narretz Narretz modified the milestones: 1.6.8, 1.6.9 Jan 4, 2018
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parentNode is buggy in Edge as well but if our tests pass then I guess we may leave it as it is. LGTM.

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but the config.browserNoActivityTimeout change should perhaps land in a separate commit.

In some circumstances, Edge 16 will not throw on clobbered
elements, because the nextSibling property is null. The exact
cause is currently unknown.
Also reduce karma log level on Travis to INFO.
Before, the log level was DEBUG, but it seems that
prior to karma 2.0.0, the debug messages were not outoput on Karma,
so this simply restores the status quo (and prevents cluttering the log).
@Narretz Narretz merged commit 8da3aef into angular:master Jan 8, 2018
@Narretz Narretz deleted the chore-edge-test branch January 8, 2018 09:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants